Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

testcase for broken Header\AbstractAddressList::fromString #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented May 18, 2017

Problem:

  1. GenericHeader loads the header in file, decodes it to utf-8
  2. the Headers::get attempts to Lazy-Load "To" header class

Lazyload does stringify and load in from string

$encoding = $current->getEncoding();
$headers  = $class::fromString($current->toString());

However, toString does not encode comma
AND To header class does split on comma!

see \Zend\Mail\Header\AbstractAddressList::fromString

this PR shows only the problem.

@glensc
Copy link
Contributor Author

glensc commented May 18, 2017

toString invocation path:

glensc added a commit to glensc/zend-mime that referenced this pull request May 18, 2017
this does solve GenericHeader::toString + Header\AbstractAddressList::fromString problem
reported here: zendframework/zend-mail#146
@glensc
Copy link
Contributor Author

glensc commented May 18, 2017

perhaps the fix is in zend-mime project:
zendframework/zend-mime#26

if it gets accepted!

@glensc
Copy link
Contributor Author

glensc commented May 18, 2017

after zendframework/zend-mime#26 being applied,
such code with Zend\Mail:

$headerLine = 'To: "=?iso-8859-1?Q?W=2C_bj=F8rn?=" <[email protected]>';
echo Mail\Header\GenericHeader::fromString($headerLine)->toString();

before:

To: =?UTF-8?Q?"W,=20bj=C3=B8rn"=20<[email protected]>?=

now:

To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"=20<[email protected]>?=

and

// with headerline without comma:
$headerLine = 'To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"=20<[email protected]>?=';
echo Mail\Header\To::fromString($headerLine)->toString();
To: =?UTF-8?Q?"W=2C=20bj=C3=B8rn"?= <[email protected]>

glensc added a commit to glensc/eventum that referenced this pull request Jul 20, 2017
got totally drowned in zend/mail, zend/mime, and eventum bugs

zendframework/zend-mime#26
zendframework/zend-mail#146
@glensc glensc mentioned this pull request Dec 13, 2017
@glensc
Copy link
Contributor Author

glensc commented Dec 13, 2017

@weierophinney closed this in zendframework/zend-mime@73e6d05 15 days ago

this is not correct. reopen please.

@weierophinney
Copy link
Member

@glensc I think I'm not understanding something.

The original string includes an encoded comma (=2c).

Why should the comma NOT be encoded when the To header is cast to a string? Is it because of the different encodings (iso-8859-1 vs UTF-8)?

(I need to understand why your expectation should be the expected behavior, basically.)

@glensc
Copy link
Contributor Author

glensc commented Jun 6, 2018

@weierophinney it's combination of these two statements from Pull Description:

  • However, toString does not encode comma
  • AND To header class does split on comma!

i tried to explain the problem in test comments as well: https://github.com/zendframework/zend-mail/pull/146/files

if my explanation is not understandable (not sure i catched your question), just see the test code and how it behaves.

it's a lot of information and i dealed with this problem more than year ago....

so, this seemed the simplest way to solve the problem. it's not forbidden to zealously encode

as zendframework/zend-mime@73e6d05 was accepted in zendframework/zend-mime#26

this just updates unit test data.

$genericHeader = Mail\Header\GenericHeader::fromString('To: "=?iso-8859-1?Q?W=2C_bj=F8rn?=" <[email protected]>');
$this->assertEquals('"W, bjørn" <[email protected]>', $genericHeader->getFieldValue());

$headers->addHeader($genericHeader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case doesn't make sense given the assertions in your comments and the issue description.

You're making the case that AbstractAddressList::fromString() is splitting on a comma, but that toString() on such headers is not encoding it.

What I'm seeing here is quite different:

  • You're testing the behavior of a GenericHeader, not one that is based on AbstractAddressList.
  • Your assertion is that the expected behavior of toString() is NOT to encode the comma.

That's the crux of my confusion: the test is not setting up the conditions you describe, nor testing the expectation you describe.

Can you please clarify?

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-mail. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-mail to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-mail.
  • In your clone of laminas/laminas-mail, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-mail; a new issue has been opened at laminas/laminas-mail#44.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants